`Callback` as `Asset` #10711

asafigan · 2023-11-23T14:22:00Z

Objective

Make managing one shot systems easier.
Addresses concerns brought up in #10582 and #10426.

Solution

Models Callbacks as Assets. This allows calling systems through Handles.

This is largely pulled from an implementation from my own game and I'm happy with the ergonomics.

This approach has many advantages:

can be created without &mut World
Systems are automatically cleaned up when no more strong Handles exist.
Systems could be loaded in using asset loaders (I have already started experimenting with this).
It seems like the future scenes will be able to embed assets. This will allow for embedding Callbacks which will be helpful if Callbacks are used with UI.

But does have some disadvantages:

The inputs and outputs of Callbacks must implement TypePath.
- This requirement comes from the Asset trait.
- This could be surprising to users especially because I don't think most users know about this trait (I didn't until I implemented this).
- Luckily this is easily derived. But can be bad when using types from other crates.
When using a new type Callback (new combination of input and output), the user must call app.init_asset::<Callback<I, O>>().
&mut Assets<Callback<I, O>> is required to create a Handle. This a double edge sword.
- this allows in-lining callback creation in spawn calls. But you can't in-line the creation of a callback in another callback.
- Different types of Callbacks require different Assets types to create Handles.

Alternatives explored before landing on this implementation:

Using SystemId internally in Callback
- made the implementation more complicated because the requirement for &mut World to create SystemId
- used Command to create them and a Mutex to update the internal state of the Callback once the Command was run
Using internal Arc<Mutex<...>> in Callback and not use Handles.
- This was the simplest implementation.
- Made creating, cloning, and calling Callbacks extremely easy.
- Downside was that it would be really hard to allow sharing of Callbacks in Scenes.

I'm creating this as a Draft to gather feedback and see if others think that the tradeoffs and implementation make sense.

TODO:

Example
Docs
More Tests?
Changelog

Changelog

TODO

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

What changed as a result of this PR?
If applicable, organize changes under "Added", "Changed", or "Fixed" sub-headings
Stick to one or two sentences. If more detail is needed for a particular change, consider adding it to the "Solution" section
- If you can't summarize the work, your change may be unreasonably large / unrelated. Consider splitting your PR to make it easier to review and merge!

github-actions · 2023-11-23T14:25:01Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

alice-i-cecile · 2023-11-23T14:28:13Z

I'm really excited about this idea: this matches up with how I think systems-from-disk should work, and is great for enabling behaviorful objects on disk, both in UI and in the context of things like scripted objects (doors, traps,...).

Before we merge this, definitely want a strong example and ideally some module-level docs.

alice-i-cecile · 2023-11-23T14:29:46Z

crates/bevy_ecs/src/system/system_registry.rs

@@ -3,15 +3,41 @@ use crate::system::{BoxedSystem, Command, IntoSystem};
 use crate::world::World;
 use crate::{self as bevy_ecs};
 use bevy_ecs_macros::Component;
+use bevy_reflect::TypePath;
 use thiserror::Error;

 /// A small wrapper for [`BoxedSystem`] that also keeps track whether or not the system has been initialized.


Remember to update these docs.

asafigan · 2023-11-23T14:31:21Z

Hmm... I didn't mean to make any breaking changes. This is meant to only be additive. How do I see what they are?

alice-i-cecile

Hard to fully evaluate without docs, but initial impressions:

I don't understand why Callback stores an Option.
I don't understand why we need the extra layer of abstraction with DriveSystem.
Not sure that the RunCallback type alias will be useful.
We should compare and contrast for the asset-based approach in the one_shot_systems example, where we make our own code-only callback type.
Definitely needs tests and examples that validate this in the context of a data-driven workflow.

Architecture and core idea seem good though!

asafigan · 2023-11-23T14:44:33Z

@alice-i-cecile Thanks for the initial feedback.

The internal Option is needed because you need to remove the BoxedSystem from the world before running it and you can't remove an Asset and insert it back with the same AssetId.
DriveSystem is there because of the Option and because we can reuse some code with SystemId.
This was to mirror the SystemId implementation.

alice-i-cecile · 2023-11-23T17:49:05Z

Sounds good :) Definitely continue with this architecture then; I'm sure it'll be clearer as it gets better documented.

alice-i-cecile · 2023-11-23T17:50:18Z

Yeah, this doesn't actually appear to have any breaking changes. Initially I thought we already had a first-party Callback type. But actually, it was just defined in the one-shot-system example.

dmyyy · 2023-11-24T02:40:51Z

Changing out the one shot system example Callback for this one could serve as a good proof of concept.

asafigan · 2023-12-01T12:07:24Z

I added an example. It might be too complex but I feel like it demonstrates the feature well.

asafigan · 2023-12-02T11:14:08Z

Because this MR has landed, I was able to simplify the logic and get rid of that internal Option. I also moved Callback to the ecs crate. The asset crate just implements Asset for it and adds traits for Commands and World to make it easy to use Handle<Callback>.

One draw back is that it is no longer possible to detect recursive calls which might make it harder to debug recursive calls. I have added a test that worked with the old implementation but now fails. I feel like this implementation is far better so I'm OK with this trade off but we might want to think if we want to add some APIs to Assets that makes it possible to detect this situation. It would be pretty easy to do this for AssetId::Index but I don't see an easy way for Asset::Uuid. I guess we could also use a private resource for tracking AssetId<Callback>s that are already in being called.

asafigan · 2024-03-10T02:53:04Z

I'm unsure this is the right direction with the closure of #10582. I do agree that observers are probably the better solution. I will also include some of my thoughts about the callback approach here.

Callbacks don't automatically have a reference to the entity. The entity either needs to be passed in by the caller or one can do some funny gymnastics when creating the callback (like spawning an entity, then creating the callback, then adding the callback to the entity).
Callbacks are manually called. This means there needs to be a system to call the callbacks. The pattern that I have used is to have a system that calls callbacks when a event that references the entity is added. This feels like boiler plate.
An ideal use case for callback are ui events. So I rewrote the game menu example using callbacks and it was more code and honestly messier.
Another ideal use case for callbacks is for use in editor. But using marker components like the existing game menu example is actually easier to implement in an editor than callbacks.
The fact that callbacks are powerful is in some ways a foot gun. It would probably be better to have a system that listens to events and have most callbacks just send events. Because of this it would probably be best to find an easy way to define event triggers for ui code and editor use.
Relations (Entity-entity relations 🌈 #3742) might also be a good replacement for editor use. Setting up a Opens relationship between a pressure plate and a door would probably be better than creating a editor defined callback.
Defining observers in editor might be a possibility. They come with all the same challenges as editor defined callbacks. Marker components, relations, or just some way of sending events (which might use observers in its implementation) would probably be a better solution for defining behavior in editor.

alice-i-cecile added the A-UI Graphical user interfaces, styles, layouts, and widgets label Nov 23, 2023

alice-i-cecile reviewed Nov 23, 2023

View reviewed changes

alice-i-cecile requested changes Nov 23, 2023

View reviewed changes

alice-i-cecile removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 23, 2023

asafigan added 3 commits December 2, 2023 19:26

callback as asset

eba64a2

add callbacks example

225e793

add callback test for detecting recursion

16d6760

asafigan force-pushed the callback branch from 6d713cc to 16d6760 Compare December 2, 2023 10:26

simplify callback logic using remove_untracked

acf138e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Callback` as `Asset` #10711

`Callback` as `Asset` #10711

asafigan commented Nov 23, 2023 •

edited

Loading

github-actions bot commented Nov 23, 2023

alice-i-cecile commented Nov 23, 2023

alice-i-cecile Nov 23, 2023

asafigan commented Nov 23, 2023

alice-i-cecile left a comment

asafigan commented Nov 23, 2023 •

edited

Loading

alice-i-cecile commented Nov 23, 2023

alice-i-cecile commented Nov 23, 2023

dmyyy commented Nov 24, 2023

asafigan commented Dec 1, 2023

asafigan commented Dec 2, 2023

asafigan commented Mar 10, 2024

Callback as Asset #10711

Are you sure you want to change the base?

Callback as Asset #10711

Conversation

asafigan commented Nov 23, 2023 • edited Loading

Objective

Solution

Changelog

github-actions bot commented Nov 23, 2023

alice-i-cecile commented Nov 23, 2023

alice-i-cecile Nov 23, 2023

Choose a reason for hiding this comment

asafigan commented Nov 23, 2023

alice-i-cecile left a comment

Choose a reason for hiding this comment

asafigan commented Nov 23, 2023 • edited Loading

alice-i-cecile commented Nov 23, 2023

alice-i-cecile commented Nov 23, 2023

dmyyy commented Nov 24, 2023

asafigan commented Dec 1, 2023

asafigan commented Dec 2, 2023

asafigan commented Mar 10, 2024

`Callback` as `Asset` #10711

`Callback` as `Asset` #10711

asafigan commented Nov 23, 2023 •

edited

Loading

asafigan commented Nov 23, 2023 •

edited

Loading